Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nixfying #536

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Nixfying #536

wants to merge 11 commits into from

Conversation

countoren
Copy link

No description provided.

@vigoo vigoo requested a review from afsalthaj May 28, 2024 09:33
@afsalthaj
Copy link
Contributor

Thanks @countoren for nixifying 👍

I am testing your branch right now, but bumping into some errors.

nix-shell
error:while calling a functor (an attribute set with a '__functor' attribute)

         at /Users/afsalthaj/github/nix/golem/default.nix:9:1:

            8| }:
            9| pkgs.rustPlatform.buildRustPackage {
             | ^
           10|   pname = "golem-cli";

       error: assertion '((cargoVendorDir == null) -> (! ((cargoSha256 == "") && (cargoHash == ""))))' failed

       at /nix/store/01sqyyi3b57wh47aax0mzyidyzg08qsk-nixpkgs-21.05pre273666.d1257435332/nixpkgs/pkgs/build-support/rust/default.nix:58:1:

           57|
           58| assert cargoVendorDir == null -> !(cargoSha256 == "" && cargoHash == "");
             | ^
           59| assert buildType == "release" || buildType == "debug";

@afsalthaj
Copy link
Contributor

Any updates on this @countoren ? We would like to get this in, just that I was having trouble testing this branch. #536 (comment)

@countoren
Copy link
Author

Hi @afsalthaj,

I missed the first comment,
did you try:

nix develop

instead of nix-shell

@countoren
Copy link
Author

I think i was able to replicate the issue I will try to fix it this weekend.

@countoren
Copy link
Author

countoren commented Sep 7, 2024

image
i was able to nix-build on my end but I had an issue with nix build (flake)
this might mean that your local nixpkgs might refer to a set that is not compatible I will try to fix the nix build flake one
to have it on a "locked" packages as well from your side.

@countoren
Copy link
Author

countoren commented Sep 8, 2024

Just add missing explicit import
tested it here on a fresh cloned repo. test now if you can :

nix build

to get the a result with golem cli
and

nix develop

to jump into the dev shell

@afsalthaj
Copy link
Contributor

@countoren Thank you for this. I will try nix soon and if works, we will get it merged.

@afsalthaj
Copy link
Contributor

afsalthaj commented Sep 18, 2024

It hasn't worked for me. I have not been following nix for more than a year. That said, it's going to be harder for developers and maintain this, as it seems it may work in 1 machine and not in the other.


error: flake 'git+file:///..../nixed/golem' does not provide attribute 'devShells.x86_64-darwin.default', 'devShell.x86_64-darwin', 'packages.x86_64-darwin.default' or 'defaultPackage.x86_64-darwin'
       Did you mean devShells?

@afsalthaj
Copy link
Contributor

@countoren Nix-shell worked for me

'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/ast.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/ast.rs'
'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/validate.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/validate.rs'
'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/parser.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/parser.rs'
'/nix/store/ma6za5vicwdys9jr5qbfpfc3m88z0j9w-wasmtime-c2e97ff/crates/wasi-common/WASI/tools/witx/src/phases.rs' -> '/nix/store/bdsf6ihs4yqfq5zyrikf2hjf7mdvyyc1-witx-0.9.1/src/phases.rs'
building '/nix/store/k0g60rs0b0vc68fakhbjnaz3a19cw3vl-cargo-vendor-dir.drv'...

[nix-shell:~/projects/nixed/golem]$

@afsalthaj
Copy link
Contributor

[nix-shell:~/projects/hello/nixed/golem]$ which cargo
/nix/store/xggv43lp8l2nq5qhgpzihiwf1dsw9nl0-auditable-cargo-1.80.1/bin/cargo

Not sure rustup is still pointing to my own installation.

So in short, nix-shell finally worked for me, and not nix-develop

@@ -0,0 +1,10529 @@
# This file is automatically @generated by Cargo.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is generating this copy of our Cargo.lock file and what happens when we change our dependencies (which is quite frequent?)

, golem-examples ? pkgs.fetchFromGitHub {
owner = "golemcloud";
repo = "golem-examples";
rev = "5165dcc7e3cbfa09f752caa96a869d284ec169aa";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this revision? What happens when we update golem-exampels? (we do it quite frequently)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this revision is just for "testing" this parameter will be overwritten by flake, this is used in nix-build/nix-shell only
flake suppose to be the default use (nix build) look at the comment at the flake inputs to see how to keep update.

}:
pkgs.rustPlatform.buildRustPackage {
pname = "golem-cli";
version = "0.0.98";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outdated now - who and when would update it? We are not keeping our version numbers anywhere else in our repo but use git tags to do releases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be set to 0.0.0, I guess it will be possible to set it in a shellScript command before build from a tag but that will count as kinda of "side effect" and then you will have issue for people that will want to run:

nix build github:golemcloud/golem

without cloning it.

cargoLock = {
lockFileContents = builtins.readFile ./Cargo.lock.nix;
allowBuiltinFetchGit = false;
outputHashes = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is special about these two dependencies? What if they need to be updated? Or how do we know we need them or need something else here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not 100% sure why nix could not include this dependencies directly from lock file, I had to include them explicitly to resolve some conflicts.
in order to update them it could be done manually if they are not updated that frequently otherwise it is possible to add another command to update-deps command in commands.nix (dev shell)

)
.unwrap();

- println!("cargo::rerun-if-changed=build.rs");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file tells me that this nix package is trying to build Golem with an older rust version (the :: syntax is since 1.77). We don't want to pin our codebase to an old rust version, and are potentially using new rust features so even though we could change this particular syntax back but the build can break any time if it's not using the same rust version that we are using (which is, always the latest, for now).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to run an update (remove/update flake.lock) and update the dependencies we might wont need this line anymore.

@@ -0,0 +1,37 @@
{
inputs.golem-examples.url = "github:golemcloud/golem-examples";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is golem-examples special for nix, and not our other dependencies? (For example golem-wasm-rpc, golem-wasm-ast, golem-wit, etc)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had issue building golem-examples when building golem-cli from nix
I think it was more "side effecty" setup.rs then the other projects, it was missing "Inflector" package. I was able to solve it referencing it on the nix build of golem-cli.
update-deps in the dev shell (commands.nix) will update it to latest version

@@ -0,0 +1,82 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a copy of our golem-cli cargo file here? Who and when will keep it updated? What is the difference?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the difference between this file and Cargo.toml is the change to of golem examples to be to one that is brought by nix (inputs.golem-examples, and additional missing reference),
I think I had planned to be included in the update-deps command on the commands.nix but i might missed something I could look at it this weekend.

name = "golem-cli"
version = "0.0.0"
dependencies = [
+ "Inflector",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this dependency added by a patch here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had to include it to build correctly golem-examples but I might be wrong I will check it this weekend

@countoren
Copy link
Author

It hasn't worked for me. I have not been following nix for more than a year. That said, it's going to be harder for developers and maintain this, as it seems it may work in 1 machine and not in the other.


error: flake 'git+file:///..../nixed/golem' does not provide attribute 'devShells.x86_64-darwin.default', 'devShell.x86_64-darwin', 'packages.x86_64-darwin.default' or 'defaultPackage.x86_64-darwin'
       Did you mean devShells?

it was seatching for devShell did you try nix build?

@marijanp
Copy link

marijanp commented Sep 24, 2024

@countoren @vigoo I have been packaging golem not being aware of this branch and I have proposed some changes to upstream golem-wasm-rpc and golem-examples to allow specifying the paths through an environment variable.

The package expression can be found here NixOS/nixpkgs#344289

Having golem in nixpkgs would also not require you to maintain Nix inside of this repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants